Skip to content

ci(deploy): still probe chat when the rollout gate fails - #1961

Merged
lilyshen0722 merged 2 commits into
mainfrom
ci/deploy-chat-probe-on-failed-rollout
Sep 28, 2026
Merged

lilyshen0722 merged 2 commits into
mainfrom
ci/deploy-chat-probe-on-failed-rollout

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

What

The chat probe runs when the rollout gate fails, not only when every step before it succeeded.

  • Verify rollout of the deployed workloads gets id: rollout.
  • Verify the new backend serves chat gets if: ${{ success() || steps.rollout.outcome == 'failure' }}.

Why

Measured on the deploy that hung (Deploy Dev run 36313468431, image 4e60f240, 2026-09-27): kubectl rollout status "$d" --timeout=8m fails → the step exit 1s at line 227 inside itself → the job stops there → the chat-probe step at line 238 is skipped. The only two if: always() steps in the job are the notice and the outcome report below it.

So the operator gets backend did not become ready within 8m and nothing else, while the step that does not run carries the one line that would explain it:

::error title=Chat unavailable on the new backend::$POD has pg/status=$STATUS and pg/messages=$ROUTE (401 means mounted, 404 unmounted)

That 401/404 is the discriminator between readiness is lying about a pod that is serving and the pod is genuinely broken — and the first is the exact shape of the #1901/#1958 family (Sentry wrapping the layer handle made the mount probe answer false, so /ready refused a pod whose /api/pg/messages was mounted). A stuck rollout arrives with its own cause attached instead of a generic one, and the diagnosis that cost time that night is available from the job log.

Both facts re-read from the workflow, not inferred: the exit 1 is at line 227 of the rollout step; the probe at line 238 has no if:; the if: always() at line 267 belongs to the notice step.

How, and why not always()

if: ${{ success() || steps.rollout.outcome == 'failure' }} — verified from the workflow, not taken on trust.

Deliberately not always() / !cancelled(). Those also run the probe when an earlier step failed — image push, Helm upgrade, GKE credentials — where the cluster may be unreachable or unchanged and the newest backend pod is the old one. The probe would then print Chat unavailable on the new backend for a reason that has nothing to do with chat, i.e. it would trade a generic-but-true error for a specific-but-false one. Condition on the step that actually matters.

Safety

This cannot turn a passing deploy red:

state before the probe behaviour
everything succeeded unchanged (runs, as today)
an earlier step failed skipped (unchanged)
the rollout gate failed new: runs, prints pg/status + pg/messages per attempt

The added case is output on a job that has already failed, so its exit code changes nothing; the worst case is the 6×10s exec retry loop on an already-red job. If the stuck pod is the newest one and cannot exec at all, the loop still ends in the named error rather than a false "serves chat".

Verification

  • YAML.load_file parses the file; both steps read back with the expected id / if.
  • No test in the repo consumes this workflow (grepped).
  • Not exercised end-to-end here — that needs a failing rollout in the cluster. The semantic relied on is explicit: the expression contains a status check function, so the implicit success() gate does not apply and the disjunction governs.

Companion row: TASK-173. Found while gating #1958 (sprint-review's observation, verified against the workflow here).

The agent-visible bad deploy right now is readiness refusing a pod that is in
fact serving: Sentry wrapping the express layer handle made the mount probe
answer false, so /ready refused a pod whose /api/pg/messages was mounted, the
new pod never became Ready, and `kubectl rollout status --timeout=8m` failed
the job.

The step that carries the diagnosis does not run in that case. The rollout
step exit 1s at line 227 inside itself, so the job stops there and skips
`Verify the new backend serves chat` at line 238, whose error text is the one
line that distinguishes a mounted route (401) from an unmounted one (404) —
"readiness is lying about a working pod" vs "the pod is broken". The operator
sees only "backend did not become ready within 8m".

Give the rollout step an id and run the probe when that step failed. Not
always()/!cancelled(): if an earlier step failed (image push, helm, GKE
credentials) the cluster is unreachable or unchanged and the probe would
report "Chat unavailable on the new backend" for a reason unrelated to chat.

Cannot turn a green job red: the added runs are on a job that already failed,
and the success and early-failure paths are unchanged.
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

The Service Tests (Tier 1 — real DBs) failure here is not from this diff — this PR touches one file, .github/workflows/deploy-dev.yml, and cannot reach a jest suite.

It failed at 11:33:40Z in __tests__/service/migrate-task-source-ref-identity.test.js:73, the dry-run case asserting the pair index is absent, receiving ["_id_","podId_1_assignee_1_status_1","podId_1_taskId_1","podId_1_sourceRef_1_partial","podId_1_sourceRef_1_title_1_partial"].

That suite's own process re-creates the pair index: the Tier-1 setupMongoDb dropDatabase()s and the model's queued autoIndex createIndexes then lands after the test has dropped it. Measured locally on the same sequence — autoIndex on, the pair index appears 321 ms after the read; with autoIndex off it never appears.

#1963 is the fix (it turns autoIndex off in that process, with the reasoning in the thread there). Treat this red as inherited rather than yours to fix, and re-run once #1963 has merged — the Tests workflow is green again on the migration suite at that point.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CODE GATE: PASS @ 22b6be6c — sprint-review. 1 file / +14, author Lily, mergeStateStatus: CLEAN, 14 checks pass with nothing red or pending.

Gating this unasked: it has been green and untouched since 12:25 with zero reviews, and it exists because of a finding of mine, so leaving it to stall was not a neutral choice.

Carry is a non-issue despite being 16 behind. No commit has touched .github/workflows/deploy-dev.yml since the merge-base (56f13d52), merge-tree --write-tree against current main is clean, and the diff is byte-identical to the one I read at 21:19.

The important part: nothing in CI checks this file

  • on: is workflow_dispatch only, so this workflow never runs on a pull request. gh run list --branch ci/deploy-chat-probe-on-failed-rollout --workflow=deploy-dev.yml returns no runs.
  • There is no actionlint (or any workflow linter) in .github/workflows/.
  • A malformed workflow manifests as startup_failure, which is absent from gh pr checks rather than appearing red — so a broken expression would not show up here at all, and would first be discovered at the next Deploy Dev dispatch.

The fourteen green checks are therefore silent about this change. The parse is the gate:

parsed OK   jobs=1  steps=18
step ids:   ["tag","rollout"]        duplicate ids: none
"Verify rollout of the deployed workloads"   id="rollout"   if=undefined
"Verify the new backend serves chat"         if="${{ success() || steps.rollout.outcome == 'failure' }}"
"Report deploy outcome"                      if="always()"    (unchanged)

rollout is a unique id, so steps.rollout.outcome resolves. That was the failure mode worth checking: a missing or duplicated id makes the expression evaluate to empty, the condition permanently false, and the fix inert while looking applied. The rollout step carries no if: of its own, so it still runs unconditionally, and Report deploy outcome keeps always() so outcome reporting is untouched.

The condition, in all three branches

  • an earlier step fails (image push, helm, GKE creds): the rollout step is skipped, so steps.rollout.outcome == 'skipped' ≠ 'failure' and success() is false — the probe stays skipped. This is the alarm-fatigue case, and choosing it over my suggested always() was right: always() would have reported "Chat unavailable on the new backend" whenever the cluster was simply unreachable.
  • the rollout gate fails alone: success() false, second clause true, probe runs. This is the 2026-09-27 case the comment documents.
  • the probe then exits 0: cannot un-fail the job, because the rollout step's failure already set the job's conclusion.

No findings. The change does what its comment says, and the comment records why always() was rejected — which is the part a future editor would otherwise undo.

@lilyshen0722
lilyshen0722 added this pull request to the merge queue Sep 28, 2026
Merged via the queue into main with commit bbab5c3 Sep 28, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant